fix(sweep,sync): correct optuna install hint + drop false "0 records" warning - #142
Merged
Merged
Conversation
… warning Two small SDK bugs found in a docs audit of #131: - sweep: the bayes-needs-optuna hint said `pip install pluto[sweep]`, but the distribution is `pluto-ml` and `pluto` is an unrelated package on PyPI — so following the hint silently installs someone else's package (with an unknown extra) instead of failing. Corrected to `pip install "pluto-ml[sweep]"`. - op: finish() warned "{pending} records may not have been uploaded" using a count read AFTER stop()'s SIGTERM drain had already emptied the queue, so it printed "0 records may not have been uploaded" — a data-loss-looking message at the moment nothing was lost. Only warn when pending > 0. Tests: bayes-missing-optuna message names pluto-ml (not pluto); finish() doesn't warn when the drain emptied the queue but does when records genuinely remain. Note: does NOT touch the ~30s finish stall (throttle-vs-shutdown mismatch) — a separate, more involved change, deferred. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe shutdown path now suppresses warnings when no records remain pending. The Bayesian sweep error now recommends the ChangesBehavior Corrections
Estimated code review effort: 2 (Simple) | ~10 minutes Mergeability Score: ⚪ Minimal · up to The PR corrects an installation hint and removes a misleading warning, with no actionable merge-blocking risk remaining after normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two small SDK bugs surfaced in a docs audit of #131. Both are low-risk and don't change how a normal run uploads during a run.
1. Wrong package name in the optuna install hint (
pluto/sweep.py)A missing-optuna
bayessweep raised a hint sayingpip install pluto[sweep]. But the distribution ispluto-ml, andplutois a real, unrelated package on PyPI — so following the hint silently installs someone else's package (with an unknown[sweep]extra) instead of failing loudly, leaving the user with a foreign package and still no optuna. Fixed topip install "pluto-ml[sweep]".2. Contradictory "0 records may not have been uploaded" warning (
pluto/op.py)On a large run,
finish()'s wait can time out (sync_completed=False), after whichstop()'s SIGTERM drain empties the queue.op.pythen read the pending count after that drain — so it literally printed0 records may not have been uploaded, a data-loss-looking message at the exact moment nothing was lost. Now it only warns whenpending > 0.Tests
test_bayes_missing_optuna_names_the_real_package— the hint namespluto-ml[sweep], notpluto[sweep].TestFinishDrainWarning— no warning when the drain emptied the queue; still warns when records genuinely remain.Not included (deferred)
This does not touch the underlying ~30s finish stall (a throttle-vs-shutdown mismatch that predates #131, from #27). That's a separate, more involved change that mostly affects bulk migration of large runs and warrants its own concurrency/load testing.
🤖 Generated with Claude Code
Note
Low Risk
Logging and error-message fixes only; no change to upload behavior during normal runs or sweep optimization logic.
Overview
Fixes two small SDK messaging bugs from a docs audit.
Bayes sweep install hint (
pluto/sweep.py): When optuna is missing, theImportErrornow tells users to install"pluto-ml[sweep]"instead ofpluto[sweep]. The old name pointed at a different PyPI package, so following it could install the wrong project and still leave optuna missing.Finish / sync shutdown (
pluto/op.py): If sync shutdown times out but the SIGTERM drain already emptied the queue,finish()no longer logs that records may not have been uploaded whenget_pending_count()is 0. The warning still appears when pending records remain.Tests cover the optuna message and both drain-warning cases (
TestFinishDrainWarning,test_bayes_missing_optuna_names_the_real_package).Reviewed by Cursor Bugbot for commit a2aee2d. Configure here.
Summary by CodeRabbit
Bug Fixes
Tests